Skip to content

test: producer-to-collector routing on pg_stat_ch.block_format#117

Open
JoshDreamland wants to merge 3 commits into
mainfrom
arrow_routing_test
Open

test: producer-to-collector routing on pg_stat_ch.block_format#117
JoshDreamland wants to merge 3 commits into
mainfrom
arrow_routing_test

Conversation

@JoshDreamland

Copy link
Copy Markdown
Contributor

Summary

Closes the producer-side half of the Arrow exporter migration contract: pin pg_stat_ch.block_format emission and the routing-connector match-shape via a generic TAP test.

The unified Arrow exporter (PR #116's commit 4) emits pg_stat_ch.block_format=arrow_events_raw on its OTLP envelope; the legacy ArrowBatchBuilder path emits arrow_ipc. In prod, the central datagres-otelcol deployment's routingconnector is supposed to fan batches between the legacy query_logs_arrow receiver target and a new events_raw receiver target based on that marker. This test pins the producer side of that contract: it doesn't run a real receiver (no CH, no datagres-arrow-exporter) — it runs stock otelcol-contrib with two file exporters and asserts which one received which arm's batches.

What it catches (and what it doesn't)

Catches:

  • Producer regression in pg_stat_ch.block_format emission (e.g. unified path silently falling back to arrow_ipc).
  • Routingconnector config-shape drift (attribute name typo, OTTL statement that no longer matches).
  • OTLP-side serialization regressions that make the marker unreachable to a connector matching on resource attributes.

Does NOT catch:

  • The events_raw column-set contract or the receiver's wire-format expectations — those live behind datagres-arrow-exporter in clickgres-platform and are covered by a separate receiver-faithful test there (in flight).
  • That t/036_unified_arrow_e2e.pl continues to handle the producer Arrow IPC -> direct CH ingest path.

Mechanics

  • New compose profile docker/docker-compose.arrow-route.yml (separate from the existing docker-compose.otel.yml so t/024 and t/037 can coexist; different ports, different container name).
  • New collector config docker/otel-routing/collector-config.yaml — stock otel/opentelemetry-collector-contrib:0.120.0, routingconnector keyed on the resource attribute, three file exporters (legacy / events_raw / default fall-through) writing JSONL to a bind-mounted dir.
  • t/psch.pm helpers: psch_routing_collector_available / psch_start_routing_collector / psch_stop_routing_collector.
  • t/037_arrow_routing.pl runs two arms (legacy + unified) and asserts that the corresponding file grew while the other files didn't. The unified arm also greps the routed JSONL tail for arrow_events_raw to pin the marker value content.

Stacking

Based on unified_arrow_exporter (PR #116). Hard-depends on commits 2 (the GUC) and 4 (the marker swap). Will retarget to main after #116 merges.

Test plan

  • Docker / collector image pull succeeds in CI.
  • Both arms pass with the producer-side commits already on unified_arrow_exporter.
  • Existing TAP suite still green (the new helpers + compose are additive; no shared state with psch_*_otelcol).

🤖 Generated with Claude Code

Base automatically changed from unified_arrow_exporter to main July 20, 2026 18:02
Closes the producer-side contract gap: the unified Arrow exporter emits
pg_stat_ch.block_format=arrow_events_raw on its OTLP envelope, while
the legacy ArrowBatchBuilder path emits arrow_ipc. The central OTel
collector's routingconnector is supposed to fan batches between the
legacy query_logs_arrow target and the new events_raw target based on
that marker. This test pins the producer half of the contract.

New TAP test t/037_arrow_routing.pl exercises both arms:
- arm 1 (unified=off, arrow_passthrough=on): legacy.jsonl grows,
  events_raw.jsonl and default.jsonl don't.
- arm 2 (unified=on): events_raw.jsonl grows with arrow_events_raw
  in the routed payload, legacy.jsonl and default.jsonl don't.

Stock otelcol-contrib image (matches existing t/024 setup, version
0.120.0). Routingconnector keyed on the resource attribute (where
pg_stat_ch.block_format lives via PopulateResource), two file
exporters writing JSONL to a bind-mounted dir the test reads from.
No CH receiver — the events_raw column-set contract isn't validated
here; that's covered by t/036 (producer Arrow IPC -> direct CH
ingest) and by a separate receiver-faithful test in clickgres-platform.

New helpers in t/psch.pm (psch_routing_collector_available,
psch_start_routing_collector, psch_stop_routing_collector) live on a
separate compose profile + container/ports so the new test can
coexist with t/024's collector if both fire in the same suite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 14:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new TAP test that validates the producer-side pg_stat_ch.block_format marker and an otelcol-contrib routingconnector “shape” by routing OTLP logs into different file exporters, ensuring legacy (arrow_ipc) vs unified (arrow_events_raw) batches fan out as expected during the Arrow exporter migration.

Changes:

  • Introduces a new TAP test (t/037_arrow_routing.pl) that runs legacy vs unified exporter arms and asserts which routed JSONL file grew.
  • Adds Docker Compose + collector config for a standalone routing collector using routingconnector and file exporters.
  • Extends TAP helper module (t/psch.pm) with start/stop/availability helpers for the routing collector.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
t/psch.pm Adds helper functions to manage a dedicated routing-collector docker compose deployment for the new TAP test.
t/037_arrow_routing.pl New TAP test that drives two producer configurations and asserts routing outcomes via file growth + marker grep.
docker/otel-routing/output/.gitignore Keeps routed JSONL outputs out of git while preserving the directory.
docker/otel-routing/collector-config.yaml Minimal collector pipeline using routingconnector and per-route file exporters for assertions.
docker/docker-compose.arrow-route.yml Standalone compose stack for the routing collector with distinct ports/container name from existing otelcol test stack.
Comments suppressed due to low confidence (1)

t/037_arrow_routing.pl:178

  • With cleanup handled via an END block (see earlier comment), the unconditional stop call here should be removed so the test doesn’t tear down a collector it didn’t start.
psch_stop_routing_collector();
done_testing();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread t/037_arrow_routing.pl
Comment thread t/037_arrow_routing.pl
Comment thread t/037_arrow_routing.pl
Comment thread t/037_arrow_routing.pl Outdated
Comment thread t/037_arrow_routing.pl Outdated
Comment thread t/037_arrow_routing.pl
t/013_clickhouse_tls.pl's reconnect subtest polled for the resumed
export within 30s of restarting the ClickHouse container. The
exporter's reconnect backoff (stats_exporter.cc PschGetRetryDelayMs)
is exponential — base 1s, doubling per consecutive failure, capped at
60s. Failed flush attempts start accumulating as soon as CH goes
down, well before the container finishes restarting, and the
cumulative wait before the Nth retry is the sum of all prior delays:
1, 3, 7, 15, 31, 63, 123s. A restart slow enough to rack up 5-6
failures — plausible under a loaded CI runner — pushes the
bgworker's next attempt past the 30s mark even though ClickHouse
itself came back much sooner.

Observed exactly this on CI twice in a row while rebasing PR #117
onto post-#116 main: identical assertion, identical 'got 0' failure,
while the same commit had passed cleanly on a less-loaded run days
earlier. Not a regression — the backoff constants and this test's
timeout have coexisted; it just needed a slow enough runner to
surface.

Bump to 90s, comfortably past the 6-failure (63s) cumulative-backoff
case plus round-trip margin.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 16:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

t/037_arrow_routing.pl:26

  • The header comment describes three test arms (unified off, then off+passthrough, then unified on), but the script actually runs only two arms and both set otel_arrow_passthrough=on. This mismatch can confuse future maintainers when updating the test.
#   2. Start a node configured to ship to it; first with the unified GUC OFF,
#      then OFF + with otel_arrow_passthrough so the legacy ArrowBatchBuilder
#      path fires; finally with the unified GUC ON.

t/037_arrow_routing.pl:46

  • The test always tears down the routing collector at the end, even if it was already running before the test started. This can disrupt local dev workflows or parallel runs. It’s also safer to register teardown in an END block so cleanup still happens if the test exits early.
    eval { psch_start_routing_collector() };
    if ($@) {
        plan skip_all => "Docker / routing collector not available: $@";
    }
}

t/037_arrow_routing.pl:177

  • With teardown handled conditionally in an END block, this unconditional shutdown is redundant and will still bring down a pre-existing collector when the test didn’t start it.
psch_stop_routing_collector();

Addresses review feedback (1 Copilot + 4 Cursor comments) on
t/037_arrow_routing.pl, verified live via prove against a real build
(not just code review):

- Track whether this run started the routing collector
  ($started_routing_collector, set before the eval'd start attempt so
  a partial startup still counts). Only tear it down if so — a
  developer's manually-started collector, or one left behind by a
  crashed prior run, is left alone.
- Move teardown into an END block (with 'local $?' to stop system()
  inside END from clobbering the script's exit code) so cleanup runs
  on every exit path: normal completion, plan skip_all, or a die()
  anywhere in the test body. Previously a die (e.g. the open-or-die
  below) skipped cleanup entirely, leaking the container and its fixed
  host ports (14317, 23133) -- ports t/026_arrow_dump.pl and
  t/036_unified_arrow_e2e.pl rely on being unbound.
- Replace the bare 'open ... or die' on arm 2's spot-check with a
  Test::More SKIP block gated on whether the file actually grew, so a
  growth timeout produces a clean failed assertion instead of aborting
  the script mid-run.
- Add per-arm producer-side bookkeeping (psch_wait_for_export +
  psch_get_stats asserting exported/send_failures), matching the
  existing t/024_otel_export.pl idiom, before the JSONL-growth checks.
  On failure this distinguishes 'producer never exported' from
  'producer exported but routing/collector dropped it'.
- Document the fixed-host-port single-instance-per-machine constraint
  in the header rather than parameterizing container/output naming:
  CI runs the TAP suite sequentially (prove without -j, single TAP
  job), and parameterizing names without also parameterizing the
  ports (14317/23133) wouldn't actually fix concurrent local runs
  while risking t/026 and t/036's assumption that those ports are
  unbound.

Live-run verification surfaced a genuine pre-existing race unrelated
to the above: the otelcol fileexporter writes a JSON line and its
trailing newline as separate writes, and arm 1's node can flush
trailing batches right up through stop(). If arm 2 snapshots its
baselines mid-write, those late bytes get mis-attributed. Added
wait_for_quiet() (polls combined JSONL size across all three files
until stable for 1.5s, 15s cap) before arm 2's baselines to close the
window. Confirmed via 3 live prove runs against a real build: one
initial 11/12 run caught the race, then 12/12 with the collector
auto-started (and torn down after), and 12/12 with the collector
pre-started manually (and correctly left running after).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 17:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread t/psch.pm
Comment on lines +270 to +278
system("docker compose -f $compose_file up -d") == 0
or die "Failed to start routing collector container";

for my $i (1..30) {
my $result = `curl -sf 'http://localhost:23133/' 2>/dev/null`;
return 1 if $result =~ /Server available/;
sleep(1);
}
die "Routing collector container failed to become healthy";
Comment thread t/037_arrow_routing.pl
Comment on lines +22 to +24
# Flow:
# 1. Spin up the routing collector via docker compose (otel-routing profile).
# 2. Start a node configured to ship to it, two arms:
Comment thread t/037_arrow_routing.pl
Comment on lines +197 to +200
# Let arm 1's trailing collector writes settle before snapshotting this
# arm's baselines (see wait_for_quiet).
wait_for_quiet(1.5, 15,
map { "$output_dir/$_.jsonl" } qw(legacy events_raw default));

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.

Comment thread t/037_arrow_routing.pl
# Let arm 1's trailing collector writes settle before snapshotting this
# arm's baselines (see wait_for_quiet).
wait_for_quiet(1.5, 15,
map { "$output_dir/$_.jsonl" } qw(legacy events_raw default));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quiet wait failure ignored

Medium Severity

Arm 2 calls wait_for_quiet to settle file writes but ignores its return value. If the timeout is reached and files are still growing, baselines are taken prematurely. This can lead to flaky assertions, mis-attributed bytes, or incorrect verification of routing isolation.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.

Comment thread t/037_arrow_routing.pl

my $default_after = current_size("$output_dir/default.jsonl");
is($default_after, $default_before,
'arm 1 (arrow_ipc): default.jsonl did NOT grow (no unmatched batches)');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isolation checks race async writes

Medium Severity

After wait_for_growth on the expected JSONL file, each arm immediately asserts the other sinks did not grow. The test notes the file exporter writes asynchronously, but there is no quiescence wait before those cross-file size checks, so a late write can yield a false pass on routing isolation.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants